Skip to content

{Packaging} Update Homebrew template to depend on pygments, pyyaml, six, and python-typing-extensions formulae#23849

Open
singingwolfboy wants to merge 1 commit intoAzure:devfrom
singingwolfboy:homebrew-deps-six-pyyaml
Open

{Packaging} Update Homebrew template to depend on pygments, pyyaml, six, and python-typing-extensions formulae#23849
singingwolfboy wants to merge 1 commit intoAzure:devfrom
singingwolfboy:homebrew-deps-six-pyyaml

Conversation

@singingwolfboy
Copy link
Copy Markdown
Contributor

@singingwolfboy singingwolfboy commented Sep 12, 2022 •
edited
Loading

Related command

Description
Change the Homebrew formula to depend on the pygments, pyyaml, six, and python-typing-extensions formulae. This makes for a smaller install package for users.

See also: Homebrew/homebrew-core#110327

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change
[Component Name 2] az command b: Add some customer-facing feature

This checklist is used to make sure that common guidelines for a pull request are followed.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 12, 2022
@ghost
Copy link
Copy Markdown

ghost commented Sep 12, 2022

Thank you for your contribution singingwolfboy! We will review the pull request and get back to you soon.

@ghost
Copy link
Copy Markdown

ghost commented Sep 12, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@ghost ghost requested a review from yonzhan September 12, 2022 22:17
@ghost ghost added the Auto-Assign Auto assign by bot label Sep 12, 2022
@ghost ghost assigned jiasli Sep 12, 2022
@ghost ghost added this to the Sep 2022 (2022-10-12) - For Ignite milestone Sep 12, 2022
@ghost ghost requested a review from jsntcy September 12, 2022 22:17
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Sep 12, 2022

Packaging

chenrui333
chenrui333 previously approved these changes Sep 16, 2022
View reviewed changes
@bebound
Copy link
Copy Markdown
Contributor

bebound commented Sep 22, 2022

Thanks for you PR. However, the dependency version these packages provide is inconsistent with requirements. I don't think it's a good idea.

@singingwolfboy
Copy link
Copy Markdown
Contributor Author

singingwolfboy commented Sep 22, 2022

Thanks for you PR. However, the dependency version these packages provide is inconsistent with requirements. I don't think it's a good idea.

It's difficult to find where these requirements are actually specified in the azure-cli repository. As you can see from the related pull request in the Homebrew repository, the actual PyPI resources currently used in the Homebrew formula are PyYAML==6.0 and six==1.14.0. The pyyaml formula in Homebrew installs PyYAML==6.0 from PyPI, so there is no inconsistency whatsoever with that change -- the actual installed code is identical to what Homebrew users have right now.

I agree that there is a very slight inconsistency with six; the six formula in Homebrew installs six==1.16.0 from PyPI, which is the latest stable version. However, there are two reasons why I believe this change is harmless, perhaps even beneficial:

  • The six changelog reports no changes in functionality between these two versions, only increased optimization and compatibility.
  • azure-cli does not even use six directly; six seems to be only used by the dependencies of azure-cli. Considering that, it's safer to make sure we are installing the latest stable version, which is what most packages would probably expect. (Or allow those dependencies to specify their own preferred version of six, instead of azure-cli specifying for them?)

I just created another pull request to change this project to use six==1.16.0. I believe that merging both of these is a much better solution than rejecting either of them.

@singingwolfboy singingwolfboy force-pushed the homebrew-deps-six-pyyaml branch from e55364e to 4def38b Compare September 25, 2022 09:52
@singingwolfboy singingwolfboy changed the title {Packaging} Update Homebrew template to depend on six and pyyaml formulas {Packaging} Update Homebrew template to depend on six, pyyaml, and python-typing-extensions formulae Sep 25, 2022
@singingwolfboy
Copy link
Copy Markdown
Contributor Author

singingwolfboy commented Sep 25, 2022

Update: the typing_extensions package now has its own Homebrew formula as well, called python-typing-extensions. I've updated this pull request to depend on this formula, as well. The version is identical, just like with the PyYAML dependency.

@SMillerDev
Copy link
Copy Markdown

SMillerDev commented Nov 3, 2022

@singingwolfboy I just saw a new release of the CLI, might be worth a rebase.

@singingwolfboy singingwolfboy force-pushed the homebrew-deps-six-pyyaml branch from 4def38b to b78ed49 Compare November 21, 2022 15:48
@singingwolfboy singingwolfboy changed the title {Packaging} Update Homebrew template to depend on six, pyyaml, and python-typing-extensions formulae {Packaging} Update Homebrew template to depend on pygments, pyyaml, six, and python-typing-extensions formulae Nov 21, 2022
@jiasli jiasli removed their assignment Dec 1, 2022
@singingwolfboy singingwolfboy force-pushed the homebrew-deps-six-pyyaml branch from b78ed49 to 618c270 Compare December 2, 2022 23:07
@chenrui333
Copy link
Copy Markdown
Contributor

chenrui333 commented Dec 11, 2022

Any update on this?

@yonzhan yonzhan requested a review from bebound December 11, 2022 01:52
@singingwolfboy singingwolfboy mentioned this pull request Dec 21, 2022
Closed
6 tasks
@jiasli jiasli modified the milestones: Feb 2023 (2023-03-07), Backlog Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kairu-ms kairu-ms Awaiting requested review from kairu-ms

@jiasli jiasli Awaiting requested review from jiasli jiasli is a code owner

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007 wangzelin007 is a code owner

@calvinhzy calvinhzy Awaiting requested review from calvinhzy

@yonzhan yonzhan Awaiting requested review from yonzhan yonzhan is a code owner

@jsntcy jsntcy Awaiting requested review from jsntcy

@bebound bebound Awaiting requested review from bebound bebound is a code owner

1 more reviewer

@chenrui333 chenrui333 chenrui333 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@bebound bebound

@jiasli jiasli

Labels

act-platform-engineering-squad Auto-Assign Auto assign by bot customer-reported Issues that are reported by GitHub users external to the Azure organization. Installation Packaging

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

7 participants

@singingwolfboy @yonzhan @bebound @SMillerDev @chenrui333 @jiasli @a0x1ab